Use fast invoke js extension methods in sync calls (#9917)#12426
Open
msynk wants to merge 21 commits into
Open
Use fast invoke js extension methods in sync calls (#9917)#12426msynk wants to merge 21 commits into
msynk wants to merge 21 commits into
Conversation
msynk
requested review from
Cyrus-Sushiant,
mhrastegari,
yasmoradi and
zahra-yousefi
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMigrates many JS interop call sites to FastInvoke, centralizes fast-path error reporting and runtime validity checks, hardens TypeScript/DOM interactions, refactors Extras resource loading, adjusts component null-handling and lifecycle disposal, and adds scanner and runtime contract tests. ChangesFastInvoke JS Interop Migration and Resilience
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cs (1)
7-7:⚠️ Potential issue | 🟡 MinorMigrate the remaining
BitBlazorUI.Utilswrappers toFastInvoke(they’re synchronous)
src/BlazorUI/Bit.BlazorUI/Scripts/Utils.tsimplementsgetBodyWidth,setProperty,getProperty,getBoundingClientRect,scrollElementIntoView,selectText, andsetStyleas non-asyncfunctions that directly return values /void(no Promise usage). Update the correspondingUtilsJsRuntimeExtensions.csmethods to usejsRuntime.FastInvoke/FastInvokeVoidinstead ofInvoke/InvokeVoidfor consistency withBitUtilsToggleOverflow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cs` at line 7, The wrappers in UtilsJsRuntimeExtensions.cs that call BitBlazorUI.Utils should be converted from synchronous jsRuntime.Invoke/InvokeVoid to the FastInvoke/FastInvokeVoid helpers used by BitUtilsToggleOverflow; locate the methods that call "BitBlazorUI.Utils.getBodyWidth", "setProperty", "getProperty", "getBoundingClientRect", "scrollElementIntoView", "selectText" and "setStyle" and replace Invoke<T>/Invoke with jsRuntime.FastInvoke<T>/FastInvoke or InvokeVoid with FastInvokeVoid while preserving argument lists and return types so the calls remain synchronous and consistent with the non-async implementations in Utils.ts.src/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.ts (1)
15-15:⚠️ Potential issue | 🟠 MajorType contract mismatch:
setupparameter should accept nullabledotnetObj.The C# side declares
DotNetObjectReference<BitPullToRefresh>?as nullable, but the TSsetupmethod parameter is typed as requiredDotNetObject. The method will crash at runtime when callingdotnetObj.invokeMethodAsync()(lines 38, 64, 71, 75) if null is passed.While the current call site (BitPullToRefresh.razor.cs:148) always creates a non-null reference, the type contract allows null and should be enforced defensively.
Recommended fix: Make the parameter optional and add a null guard:
dotnetObj: DotNetObject | undefined) { if (!dotnetObj) return;Alternatively, change the C# signature to use non-nullable
DotNetObjectReference<BitPullToRefresh>if null should never be passed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.ts` at line 15, The setup function's dotnetObj parameter in BitPullToRefresh.ts is declared non-nullable but the C# contract allows null; update the setup signature (in the setup function) to accept DotNetObject | undefined (or optional) and add an early null guard (if (!dotnetObj) return;) before any calls to dotnetObj.invokeMethodAsync() (these occur in the setup function and in any helper closures referenced there such as the touch/move handlers), or alternatively change the C# DotNetObjectReference<BitPullToRefresh> to be non-nullable if null will never be passed.
🧹 Nitpick comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.ts (1)
27-29: ⚡ Quick winRemove redundant
!element.stylecheck.The
!element.stylecondition is unreachable becauseHTMLElementalways has astyleproperty. Once the!elementcheck passes,element.styleis guaranteed to exist.♻️ Simplify the guard clause
- public static resetPaneDimensions(element: HTMLElement | undefined) { - if (!element || !element.style) return; - + public static resetPaneDimensions(element: HTMLElement | undefined) { + if (!element) return; +🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.ts` around lines 27 - 29, The guard in BitSplitter.resetPaneDimensions redundantly checks !element.style after confirming element exists; remove the unreachable "!element.style" condition so the guard becomes "if (!element) return;" and update any surrounding comments accordingly, referencing the public static method resetPaneDimensions on BitSplitter to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cs`:
- Around line 20-23: BitPdfReaderRenderPage and BitPdfReaderRefreshPage are
still calling the old InvokeVoid; change both to use FastInvokeVoid for
consistency with BitPdfReaderDispose (which already uses FastInvokeVoid). Locate
the methods BitPdfReaderRenderPage and BitPdfReaderRefreshPage and replace
jsRuntime.InvokeVoid("BitBlazorUI.PdfReader.renderPage", ...) and
jsRuntime.InvokeVoid("BitBlazorUI.PdfReader.refreshPage", ...) with
jsRuntime.FastInvokeVoid("BitBlazorUI.PdfReader.renderPage", ...) and
jsRuntime.FastInvokeVoid("BitBlazorUI.PdfReader.refreshPage", ...) respectively,
leaving BitPdfReaderSetup (Invoke<int>) unchanged.
---
Outside diff comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.ts`:
- Line 15: The setup function's dotnetObj parameter in BitPullToRefresh.ts is
declared non-nullable but the C# contract allows null; update the setup
signature (in the setup function) to accept DotNetObject | undefined (or
optional) and add an early null guard (if (!dotnetObj) return;) before any calls
to dotnetObj.invokeMethodAsync() (these occur in the setup function and in any
helper closures referenced there such as the touch/move handlers), or
alternatively change the C# DotNetObjectReference<BitPullToRefresh> to be
non-nullable if null will never be passed.
In `@src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cs`:
- Line 7: The wrappers in UtilsJsRuntimeExtensions.cs that call
BitBlazorUI.Utils should be converted from synchronous
jsRuntime.Invoke/InvokeVoid to the FastInvoke/FastInvokeVoid helpers used by
BitUtilsToggleOverflow; locate the methods that call
"BitBlazorUI.Utils.getBodyWidth", "setProperty", "getProperty",
"getBoundingClientRect", "scrollElementIntoView", "selectText" and "setStyle"
and replace Invoke<T>/Invoke with jsRuntime.FastInvoke<T>/FastInvoke or
InvokeVoid with FastInvokeVoid while preserving argument lists and return types
so the calls remain synchronous and consistent with the non-async
implementations in Utils.ts.
---
Nitpick comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.ts`:
- Around line 27-29: The guard in BitSplitter.resetPaneDimensions redundantly
checks !element.style after confirming element exists; remove the unreachable
"!element.style" condition so the guard becomes "if (!element) return;" and
update any surrounding comments accordingly, referencing the public static
method resetPaneDimensions on BitSplitter to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 73675b8f-81fa-4452-afd7-53844f2dc3f3
📒 Files selected for processing (20)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AppShell/BitAppShellJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/JsInterop/BitChartJsInterop.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGridJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/InfiniteScrolling/BitInfiniteScrollingJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/OtpInput/BitOtpInputJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBoxJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiperJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModalJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.tssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitterJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.tssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cs`:
- Line 25: The JS interop helper BitUtilsGetBoundingClientRect in
UtilsJsRuntimeExtensions currently returns
jsRuntime.FastInvoke<BoundingClientRect>(...) which can be null; update
BitUtilsGetBoundingClientRect to guard the FastInvoke result and return a
non-null BoundingClientRect (e.g. coalesce to a new BoundingClientRect with
Left/Top/Width/Height = 0) so callers like BitColorPicker.UpdateColor and
BitCircularTimePicker.UpdateTime can safely read properties without null checks
or NREs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 401888d2-353d-481f-b50a-076162576f0f
📒 Files selected for processing (4)
src/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.tssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cs
- src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.ts
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.ts`:
- Line 42: The current detection of an already-loaded script in the document
(the Array.from(document.scripts).find callback that assigns existingTag) uses
s.src.includes(url), which can produce false positives; change the predicate to
a stricter match — e.g., compare the script src filename or use
s.src.endsWith(url) (or strip query/hash and compare the last path segment) so
only exact filename/ending matches are considered; update the find call that
sets existingTag accordingly to use this precise match logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bcc2884c-09ab-4c8c-a64e-75e6bb4e0ee6
📒 Files selected for processing (9)
src/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGrid.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGridJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cs (1)
15-20: ⚡ Quick winSerialize the tests that mutate
OnError.
IJSRuntimeFastExtensions.OnErroris process-global. These cases set/reset it per test, so they can race and flake if MSTest parallelization is enabled for the assembly or CI run. Mark this class[DoNotParallelize]or guard the hook with class-level synchronization.Also applies to: 76-80, 93-96, 109-112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cs` around lines 15 - 20, The tests mutate the process-global IJSRuntimeFastExtensions.OnError which can race when MSTest runs tests in parallel; update the test class to prevent parallel execution by adding the MSTest DoNotParallelize attribute (apply to the test class declaration) or alternatively wrap all accesses to IJSRuntimeFastExtensions.OnError used in the tests (setup/teardown and tests referencing it at lines with ResetErrorHandler and the tests at 76-80, 93-96, 109-112) with a class-level lock to serialize access; pick one approach and apply it consistently so the OnError hook is never concurrently read/modified across tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.ts`:
- Around line 44-49: The script-matching currently reduces URLs to pathname only
via the normalize function and then compares normalize(s.src) to targetPath,
which conflates distinct origins and query strings; update the logic so
normalize preserves origin and query (or otherwise compute a full absolute URL
string) when generating targetPath and when comparing against document.scripts,
then use that full URL comparison to find existingTag (refer to the normalize
arrow function, targetPath constant and existingTag lookup) so scripts like
different CDNs or different ?v= queries are considered distinct.
In
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cs`:
- Around line 38-40: CollectAsyncJsMethods currently relies on
TsReturnsPromiseRegex to detect Promise-returning methods by body heuristics,
which yields false negatives for delegated returns; instead tighten the
convention so only explicit async functions or those with an explicit Promise
return type are treated as async. Update CollectAsyncJsMethods to stop using
TsReturnsPromiseRegex and only add methods to asyncJsMethods when the method has
the async modifier or its return annotation explicitly contains "Promise";
adjust any test expectations that assumed body-based detection and update
references to TsReturnsPromiseRegex (and related logic) accordingly.
---
Nitpick comments:
In
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cs`:
- Around line 15-20: The tests mutate the process-global
IJSRuntimeFastExtensions.OnError which can race when MSTest runs tests in
parallel; update the test class to prevent parallel execution by adding the
MSTest DoNotParallelize attribute (apply to the test class declaration) or
alternatively wrap all accesses to IJSRuntimeFastExtensions.OnError used in the
tests (setup/teardown and tests referencing it at lines with ResetErrorHandler
and the tests at 76-80, 93-96, 109-112) with a class-level lock to serialize
access; pick one approach and apply it consistently so the OnError hook is never
concurrently read/modified across tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2defda38-b1ea-4eb2-8611-b1ad9efc9060
📒 Files selected for processing (30)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AppShell/BitAppShellJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/JsInterop/BitChartJsInterop.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGrid.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGridJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/InfiniteScrolling/BitInfiniteScrollingJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/OtpInput/BitOtpInputJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBoxJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Carousel/BitCarousel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiperJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModalJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.tssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitterJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.tssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AppShell/BitAppShellTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.ts`:
- Around line 34-35: Normalize the incoming `url` to the same absolute form used
when matching existing script tags before using it as a cache key: compute the
normalized/absolute URL (the same normalization used when checking
`existingTag.src`) and use that normalized value for all accesses and
assignments to `Extras._scriptPromises` (instead of raw `url`) so lookups,
writes and deletions (the spots around `existingPromise =
Extras._scriptPromises[url]`, the existingTag checks, and the promise
set/resolve/remove locations) consistently use the same key and avoid races
where an equivalent URL misses the cache.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5c6f6c0f-1b50-4b3f-adc9-9d106c7d049a
📒 Files selected for processing (30)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AppShell/BitAppShellJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/JsInterop/BitChartJsInterop.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGrid.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGridJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/InfiniteScrolling/BitInfiniteScrollingJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/OtpInput/BitOtpInputJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBoxJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Carousel/BitCarousel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiperJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Modal/BitModalJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.tssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitterJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.tssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AppShell/BitAppShellTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BlazorUI/Bit.BlazorUI/Utils/BitPageVisibility.cs (1)
25-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard against multiple
Init()calls to prevent reference leak.If
Init()is called multiple times, a newDotNetObjectReferenceis created at line 31 without disposing the previous_dotnetObj, causing a resource leak. The_isInitializedcheck at line 27 prevents re-initialization logic, but the reference would already be leaked before the check returns.🔒 Proposed fix to dispose existing reference before creating new one
public async Task Init() { if (_isInitialized) return; _isInitialized = true; + _dotnetObj?.Dispose(); _dotnetObj = DotNetObjectReference.Create(this); await js.InvokeVoid("BitBlazorUI.PageVisibility.init", _dotnetObj); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Utils/BitPageVisibility.cs` around lines 25 - 34, Init() in BitPageVisibility creates a new DotNetObjectReference (_dotnetObj) without disposing a prior instance, causing a leak; before assigning a new DotNetObjectReference in Init(), check if _dotnetObj is non-null and call Dispose() (and set it null) to release the previous reference, then proceed to create the new DotNetObjectReference and call js.InvokeVoid("BitBlazorUI.PageVisibility.init", _dotnetObj); keep the existing _isInitialized guard logic intact.
🧹 Nitpick comments (2)
src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cs (2)
259-302: 💤 Low valueDocument the backward scanning limitations for maintainability.
IsNestedFunctionBodyOpenperforms backward scanning to detect function/closure bodies, which is inherently fragile (as noted in theSkipNonCodeBackwarddoc comment). The current implementation cannot reliably distinguish between regex literals (/pattern/) and division (a / b), and it may misclassify other constructs.While the doc comment at line 411-415 explains this risk, consider adding a specific comment near line 259 explaining:
- Why backward scanning is acceptable here (only affects secondary body scan, not primary async/Promise detection)
- What specific misclassifications could occur (regex literals, template literals with embedded code)
- That the tradeoff favors false negatives (missing some nested functions) over false positives
This will help future maintainers understand the design choice and avoid "fixing" the scanner in a way that introduces false positives.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cs` around lines 259 - 302, Add a focused inline comment above the IsNestedFunctionBodyOpen method explaining why backward scanning (via SkipNonCodeBackward and FindMatchingBackward) is acceptable here (it only affects secondary nested-body detection, not primary async/Promise detection), enumerate likely misclassifications (e.g., confusing regex literals like /pattern/ with division, template literals with embedded expressions, and other edge-case token boundaries), and state the deliberate tradeoff to prefer false negatives (missing some nested functions) over false positives so maintainers don’t attempt aggressive fixes that would introduce incorrect matches; reference the existing SkipNonCodeBackward doc comment in the note.
153-200: ⚡ Quick winReview the brace stack logic for nested function tracking.
The
ScanTopLevelReturnsmethod uses aStack<bool>to track whether each{belongs to a function body, incrementingnestedFunctionDepthwhen pushingtrue. However, the pop at line 176 doesn't verify that the closing}actually matches the opening{type – it only checksbraceStack.Count > 0. If a non-function{is followed by a function{and then two}, the depth tracking could desync.For example:
static method() { if (cond) { // push false const fn = () => { // push true, depth++ // ... }; // pop true, depth--, stack now [false] } // pop false, depth unchanged, stack now [] return fetch(url); // depth == 0, detected as top-level ✓ }This appears correct in normal cases. The risk is if the scanner misses a
{(e.g., inside a comment or string literal not fully skipped), leading to an unbalanced stack. TheSkipNonCodelogic should prevent this, but it's a subtle interaction.Consider adding an assertion or test case that validates balanced braces across various nesting scenarios (function in if-block, multiple closures, etc.) to ensure the stack never goes negative or becomes desynchronized.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cs` around lines 153 - 200, ScanTopLevelReturns can desynchronize nestedFunctionDepth when braceStack is unbalanced; ensure popping only occurs when there is a matching pushed item and validate what was popped before decrementing depth. Change the brace-close handling in ScanTopLevelReturns to check braceStack.Count > 0 before Pop (already present) but also capture the popped value (e.g., var poppedIsFunction = braceStack.Pop()) and only decrement nestedFunctionDepth when poppedIsFunction is true; additionally add assertions or unit tests exercising ScanTopLevelReturns with nested non-function and function `{` sequences (including cases with comments/strings that could confuse SkipNonCode) to verify the stack never goes negative and depth stays consistent; reference methods: ScanTopLevelReturns, braceStack, nestedFunctionDepth, SkipNonCode, IsNestedFunctionBodyOpen, SkipNonMatchingReturnExpression.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiper.ts`:
- Around line 20-48: BitSwiper.getDimensions should return null on failure
instead of an empty object: change the early-return when !root || !container to
`return null;` and in the catch block return `null` as well; ensure callers
(e.g., BitSwiper.razor.cs which already uses `dimensions?.X ?? 0`) continue to
work with nullable results and, if necessary, update any TypeScript signature or
documentation for getDimensions to indicate a nullable SwiperDimensions return.
In
`@src/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiperJsRuntimeExtensions.cs`:
- Line 12: The method BitBlazorUI.Swiper.getDimensions call in
BitSwiperJsRuntimeExtensions.cs currently returns a non-nullable
SwiperDimensions but calls jsRuntime.FastInvoke<SwiperDimensions> which can
return null; change the method signature to return ValueTask<SwiperDimensions?>
and propagate nullable handling (i.e., return the FastInvoke result as-is) so
callers can handle nulls, matching the nullable pattern used in
BitSplitterGetSplitterWidth and BitUtilsGetBodyWidth.
In `@src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cs`:
- Around line 19-21: The BitUtilsGetProperty method currently returns
ValueTask<string> but calls jsRuntime.FastInvoke<string> which can yield null;
change the method signature to return ValueTask<string?> and adjust any nullable
annotations accordingly so the method expresses the nullable contract; update
callers (or their null-handling) if they assume non-null, and keep the method
body using jsRuntime.FastInvoke<string> unchanged (only the return type and any
immediate null-related annotations/flow must be updated).
---
Outside diff comments:
In `@src/BlazorUI/Bit.BlazorUI/Utils/BitPageVisibility.cs`:
- Around line 25-34: Init() in BitPageVisibility creates a new
DotNetObjectReference (_dotnetObj) without disposing a prior instance, causing a
leak; before assigning a new DotNetObjectReference in Init(), check if
_dotnetObj is non-null and call Dispose() (and set it null) to release the
previous reference, then proceed to create the new DotNetObjectReference and
call js.InvokeVoid("BitBlazorUI.PageVisibility.init", _dotnetObj); keep the
existing _isInitialized guard logic intact.
---
Nitpick comments:
In
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cs`:
- Around line 259-302: Add a focused inline comment above the
IsNestedFunctionBodyOpen method explaining why backward scanning (via
SkipNonCodeBackward and FindMatchingBackward) is acceptable here (it only
affects secondary nested-body detection, not primary async/Promise detection),
enumerate likely misclassifications (e.g., confusing regex literals like
/pattern/ with division, template literals with embedded expressions, and other
edge-case token boundaries), and state the deliberate tradeoff to prefer false
negatives (missing some nested functions) over false positives so maintainers
don’t attempt aggressive fixes that would introduce incorrect matches; reference
the existing SkipNonCodeBackward doc comment in the note.
- Around line 153-200: ScanTopLevelReturns can desynchronize nestedFunctionDepth
when braceStack is unbalanced; ensure popping only occurs when there is a
matching pushed item and validate what was popped before decrementing depth.
Change the brace-close handling in ScanTopLevelReturns to check braceStack.Count
> 0 before Pop (already present) but also capture the popped value (e.g., var
poppedIsFunction = braceStack.Pop()) and only decrement nestedFunctionDepth when
poppedIsFunction is true; additionally add assertions or unit tests exercising
ScanTopLevelReturns with nested non-function and function `{` sequences
(including cases with comments/strings that could confuse SkipNonCode) to verify
the stack never goes negative and depth stays consistent; reference methods:
ScanTopLevelReturns, braceStack, nestedFunctionDepth, SkipNonCode,
IsNestedFunctionBodyOpen, SkipNonMatchingReturnExpression.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1d022c3a-2d6a-4923-8cf2-0013dbfd4680
📒 Files selected for processing (52)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AppShell/BitAppShellJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/JsInterop/BitChartJsInterop.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGrid.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGridJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/InfiniteScrolling/BitInfiniteScrollingJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownViewer/BitMarkdownViewerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.tssrc/BlazorUI/Bit.BlazorUI.Extras/Services/BitMarkdownService.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DatePicker/BitDatePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DateRangePicker/BitDateRangePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUpload.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/OtpInput/BitOtpInputJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBoxJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Carousel/BitCarousel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiper.tssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiperJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Dialog/BitDialog.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Panel/BitPanel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.tssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitterJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/Overlay/BitOverlay.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.tssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.tssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Swipes.tssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Bit.BlazorUI/Utils/BitPageVisibility.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Bit.BlazorUI.Tests.csprojsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AppShell/BitAppShellTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IsRuntimeInvalidFrameworkContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScannerTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cs (1)
20-43:⚠️ Potential issue | 🟡 MinorMigrate remaining synchronous JS interop to
FastInvokeVoidIn
Extras.ts,initScriptsandinitStylesheetsareasync(Promise-returning), so keepingBitExtrasInitScripts/BitExtrasInitStylesheetsonInvokeVoidis fine. However,setPreventKeys,disposePreventKeys, andscrollOptionIntoVieware synchronous (notasync), soBitExtrasSetPreventKeys,BitExtrasDisposePreventKeys, andBitExtrasScrollOptionIntoViewshould be switched toFastInvokeVoidto match the migration intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cs` around lines 20 - 43, Change the three synchronous JS interop calls to use FastInvokeVoid: in BitExtrasSetPreventKeys, BitExtrasDisposePreventKeys, and BitExtrasScrollOptionIntoView replace jsRuntime.InvokeVoid(...) with jsRuntime.FastInvokeVoid(...) while keeping the same arguments (element, keys) and (element) and (optionId) respectively so the signatures and behavior remain unchanged.
🧹 Nitpick comments (1)
src/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.razor.cs (1)
157-157: ⚡ Quick winResult is discarded on non-first render but used on first render.
Line 145 uses the JS interop result to determine chart readiness, but line 157 discards it with
_. This inconsistency suggests either:
- The result matters only on first render (current behavior), or
- The result should be checked on subsequent renders as well.
Verify that ignoring the setup result on non-first renders is intentional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.razor.cs` at line 157, The code currently discards the JS interop result when calling BitChartJsSetupChart(Config) on non-first renders; either preserve the original readiness check or make the intent explicit. Update the call in BitChart.razor.cs to capture the result (e.g., var result = await _js.BitChartJsSetupChart(Config)) and apply the same readiness logic used on first render (set the same readiness flag or call the same method that handles the result), or if ignoring the result is intentional, add a clear comment explaining why and keep the discard. Ensure you reference and update the same readiness state/flag used elsewhere in the class so behavior is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.razor.cs`:
- Around line 145-150: The code currently treats Config is null as "ready" and
invokes SetupCompletedCallback without calling the interop; change the flow so
the chart setup interop is always awaited and determines readiness: call await
_js.BitChartJsSetupChart(Config) (remove/avoid short-circuiting on Config is
null) and only set chartReady/raise SetupCompletedCallback when that interop
returns true; update the logic around the chartReady local, the call to
_js.BitChartJsSetupChart(Config), and the conditional that invokes
SetupCompletedCallback so the callback only fires after the JS setup completes.
In
`@src/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGridJsRuntimeExtensions.cs`:
- Around line 15-20: Update the explanatory comment in
BitDataGridJsRuntimeExtensions (the block describing why the code uses the
regular async IJS invocation) to remove the incorrect claim that
IJSRuntimeFastExtensions.FastInvokeVoid "only swallows JsonException" and
instead state the correct reason: FastInvokeVoid (per
IJSRuntimeFastExtensions.FastInvokeVoid) runs synchronously on WebAssembly and
can alter Promise/ordering and error propagation semantics, so we deliberately
use the async Invoke pattern to ensure that JS-side failures are contained
within the returned Task and do not escape synchronously into the render loop;
keep the rest of the explanation about DOM-heavy positioning
(getBoundingClientRect, scrollIntoViewIfNeeded, focus) intact.
In `@src/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.ts`:
- Around line 138-172: The non-complete branch of awaitHostResource delays even
when the host resource is already applied; change awaitHostResource to perform
the same immediate checks used in the document.readyState === 'complete' branch
before attaching listeners: for kind === 'stylesheet' call
Extras.isHostStylesheetApplied(element as HTMLLinkElement) and for kind ===
'script' call Extras.isHostScriptLoaded(element as HTMLScriptElement), and if
the check shows the resource is present/loaded return Promise.resolve(),
otherwise proceed to create the Promise and add the load/error/window listeners;
keep using Extras.loadResourceError(...) for rejection when checks fail.
In `@src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.ts`:
- Around line 11-19: The getSplitterWidth function should return null on
geometry-read failures instead of 0 to preserve the JS↔C# nullable contract:
change the signature to return number | null, return null in the early guard
(when !element or missing getBoundingClientRect) and return null in the catch
block rather than 0; apply the same changes to the sibling function
getSplitterHeight (update its signature to number | null and return null on
invalid element or exceptions).
In `@src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cs`:
- Around line 19-22: The BitUtilsGetProperty extension returns
ValueTask<string?> but calls jsRuntime.FastInvoke<string>; change the generic
parameter to FastInvoke<string?> to match the nullable contract (i.e., update
the call inside BitUtilsGetProperty to
jsRuntime.FastInvoke<string?>("BitBlazorUI.Utils.getProperty", element,
property)) so the method signature and invocation nullability are consistent;
ensure using IJSRuntime's FastInvoke overload that accepts the nullable generic.
---
Outside diff comments:
In
`@src/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cs`:
- Around line 20-43: Change the three synchronous JS interop calls to use
FastInvokeVoid: in BitExtrasSetPreventKeys, BitExtrasDisposePreventKeys, and
BitExtrasScrollOptionIntoView replace jsRuntime.InvokeVoid(...) with
jsRuntime.FastInvokeVoid(...) while keeping the same arguments (element, keys)
and (element) and (optionId) respectively so the signatures and behavior remain
unchanged.
---
Nitpick comments:
In `@src/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.razor.cs`:
- Line 157: The code currently discards the JS interop result when calling
BitChartJsSetupChart(Config) on non-first renders; either preserve the original
readiness check or make the intent explicit. Update the call in
BitChart.razor.cs to capture the result (e.g., var result = await
_js.BitChartJsSetupChart(Config)) and apply the same readiness logic used on
first render (set the same readiness flag or call the same method that handles
the result), or if ignoring the result is intentional, add a clear comment
explaining why and keep the discard. Ensure you reference and update the same
readiness state/flag used elsewhere in the class so behavior is consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5a130237-07d6-468a-afa9-3bb22c8ec590
📒 Files selected for processing (53)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AppShell/BitAppShellJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/JsInterop/BitChartJsInterop.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGrid.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGridJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/InfiniteScrolling/BitInfiniteScrollingJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownViewer/BitMarkdownViewerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.tssrc/BlazorUI/Bit.BlazorUI.Extras/Services/BitMarkdownService.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DatePicker/BitDatePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DateRangePicker/BitDateRangePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUpload.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/OtpInput/BitOtpInputJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBoxJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Slider/BitSlider.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Carousel/BitCarousel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiper.tssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiperJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Dialog/BitDialog.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Panel/BitPanel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.tssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitterJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/Overlay/BitOverlay.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.tssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.tssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Swipes.tssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Bit.BlazorUI/Utils/BitPageVisibility.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Bit.BlazorUI.Tests.csprojsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AppShell/BitAppShellTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IsRuntimeInvalidFrameworkContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScannerTests.cs
…k/bitframework into 9917-blazorui-fastinvoke-js
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.razor.cs`:
- Around line 140-147: The InvokeAsync call on line 140 that schedules
ResetPaneDimensionsOnVerticalChange is being discarded as a fire-and-forget
operation, which means exceptions from the BitSplitterResetPaneDimensions JS
interop calls will be unobserved and silently fail. Add proper error handling to
the InvokeAsync task by attaching a continuation (such as using .ContinueWith or
.Catch) to handle any exceptions that occur during the pane dimension reset,
ensuring failures are logged or handled appropriately rather than being silently
ignored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 66f2a46d-90ae-489c-b234-252b4efabf0e
📒 Files selected for processing (54)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AppShell/BitAppShellJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/JsInterop/BitChartJsInterop.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGrid.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGridJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/InfiniteScrolling/BitInfiniteScrollingJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownViewer/BitMarkdownViewerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PhoneInput/BitPhoneInput.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.tssrc/BlazorUI/Bit.BlazorUI.Extras/Services/BitMarkdownService.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DatePicker/BitDatePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DateRangePicker/BitDateRangePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUpload.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/OtpInput/BitOtpInputJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBoxJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Slider/BitSlider.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Carousel/BitCarousel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiper.tssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiperJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Dialog/BitDialog.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Panel/BitPanel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.tssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitterJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/Overlay/BitOverlay.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.tssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.tssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Swipes.tssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Bit.BlazorUI/Utils/BitPageVisibility.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Bit.BlazorUI.Tests.csprojsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AppShell/BitAppShellTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IsRuntimeInvalidFrameworkContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScannerTests.cs
Member
Author
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cs (1)
20-28:⚠️ Potential issue | 🔴 CriticalChange to
InvokeVoidAsyncinstead ofInvokeVoid.Both
BitExtrasInitScriptsandBitExtrasInitStylesheetscall async JavaScript functions that return Promises and must be awaited. The JavaScript implementations useawait Promise.allSettled()to ensure all resources are loaded before resolving. UsingInvokeVoid()is fire-and-forget and returns a completed ValueTask immediately, breaking the async contract that callers expect. All call sites useawait, expecting the methods to wait for resource loading to complete.Use
InvokeVoidAsync()instead, which properly awaits the JavaScript Promise and returns a ValueTask that completes only after the JS function resolves.Suggested fix
public static ValueTask BitExtrasInitScripts(this IJSRuntime jsRuntime, IEnumerable<string> scripts, bool isModule = false) { return jsRuntime.InvokeVoidAsync("BitBlazorUI.Extras.initScripts", scripts, isModule); } public static ValueTask BitExtrasInitStylesheets(this IJSRuntime jsRuntime, IEnumerable<string> stylesheets) { return jsRuntime.InvokeVoidAsync("BitBlazorUI.Extras.initStylesheets", stylesheets); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cs` around lines 20 - 28, The methods BitExtrasInitScripts and BitExtrasInitStylesheets are using InvokeVoid() which is a fire-and-forget operation that returns an immediately completed ValueTask, but the underlying JavaScript functions return Promises that must be awaited before resources are fully loaded. Replace InvokeVoid() with InvokeVoidAsync() in both method implementations to properly await the JavaScript Promise resolution and ensure callers' await statements actually wait for resource loading to complete.
🧹 Nitpick comments (4)
src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cs (1)
130-135: 💤 Low valueMinor: redundant copy to result set.
Lines 130-133 copy
promiseMethodsintoresult, butresultis empty at this point and could be eliminated by directly returningpromiseMethods. This is harmless but adds a small allocation and iteration.♻️ Simplification
- foreach (var key in promiseMethods) - { - result.Add(key); - } - - return result; + return promiseMethods;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cs` around lines 130 - 135, The foreach loop in lines 130-133 unnecessarily copies each item from `promiseMethods` into the empty `result` collection. Since `result` starts empty and is only populated with items from `promiseMethods`, eliminate this redundant loop by directly returning `promiseMethods` instead of copying its contents to `result`. This removes an unnecessary allocation and iteration while maintaining the same behavior.src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cs (2)
62-64: 💤 Low valueClass-name uniqueness assumption in normalization.
LastTwoSegmentsreduces identifiers toClass.methodform, which assumes class names are unique across the scanned TypeScript sources. If multiple classes share the same name (e.g.,Namespace1.UtilsandNamespace2.Utils), a FastInvoke call to the synchronous method in one namespace could be flagged if the same method name in the other namespace is async. Given the single-project scope and typical TypeScript structure, this is a reasonable tradeoff, but worth noting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cs` around lines 62 - 64, The LastTwoSegments method reduces identifiers to Class.method form by assuming class names are unique across TypeScript sources, which could cause false positives if multiple classes share the same name across different namespaces. Add a comment near the LastTwoSegments call explaining this assumption and documenting why it is acceptable for the current single-project scope and typical TypeScript structure, making the tradeoff explicit for future maintainers.
75-79: ⚖️ Poor tradeoffEvolution from past review: delegation gap now explicit design choice.
The past review flagged that Promise detection could miss delegated returns. The current implementation has evolved:
- Delegation IS propagated from methods with explicit
asyncor: Promise<...>annotations (see TsPromiseMethodScanner lines 101-117)- Delegation is NOT propagated from body-scanned methods (those detected via
return fetch(...)heuristics)This tradeoff is now explicitly documented in TsPromiseMethodScanner (lines 19-22) and tested (TsPromiseMethodScannerTests.cs lines 135-152). The conservative approach prevents heuristic false positives from cascading through delegation chains. If the codebase convention ensures all Promise-returning methods are explicitly marked
asyncor have: Promise<...>return types, this gap is acceptable. Otherwise, consider either removing body scanning entirely (require explicit annotations) or accepting the documented limitation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cs` around lines 75 - 79, Ensure that the design tradeoff regarding Promise delegation propagation is explicitly and clearly documented in TsPromiseMethodScanner (around lines 19-22), specifically stating that delegation is propagated from methods with explicit async or : Promise<...> annotations but NOT from body-scanned heuristics like return fetch(...). Verify that TsPromiseMethodScannerTests (around lines 135-152) includes tests that validate this behavior and demonstrate the documented limitations. Additionally, confirm that the implementation in TsPromiseMethodScanner (lines 101-117) correctly implements this explicit design choice by only propagating delegation from explicitly annotated methods while intentionally excluding heuristically-detected methods.src/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.ts (1)
26-30: ⚡ Quick winAdd context prefix to error logging for consistency.
The error log on line 29 lacks the
"BitBlazorUI.ColorPicker.dispose:"prefix used consistently inBitSwiper.ts,BitSplitter.ts, and other files in this layer, making debugging slightly harder.♻️ Suggested fix
try { bitController?.dotnetObj?.dispose(); } catch (e) { - console.error(e); + console.error("BitBlazorUI.ColorPicker.dispose:", e); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.ts` around lines 26 - 30, The console.error call in the catch block of the dispose method lacks the context prefix used consistently across other components in this layer. Add the prefix "BitBlazorUI.ColorPicker.dispose:" to the error log in the catch block (where the console.error(e) statement is located) to match the logging convention used in BitSwiper.ts, BitSplitter.ts, and other files, improving debugging clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.razor.cs`:
- Around line 150-155: The XML documentation for SetupCompletedCallback states
it fires when the chart has been setup through interop and the JavaScript object
is available, but the current implementation invokes it unconditionally on first
render regardless of whether Config is null or setup succeeded. Either move the
await SetupCompletedCallback.InvokeAsync(this) call to execute only when setup
completes successfully by placing it inside the if (Config is not null) block,
or update the XML documentation comments to accurately reflect that the callback
fires unconditionally on first render. The recommended approach is to move the
callback invocation to only fire when setup actually succeeds so that consumers
relying on this callback to signal chart readiness do not receive false
positives.
In `@src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cs`:
- Around line 98-104: Remove the null check from the IsRuntimeInvalid method
that returns true when jsRuntime is null. The method should not treat null as an
invalid runtime state; instead null should propagate naturally as a caller error
to fail fast. Reserve IsRuntimeInvalid() only for detecting real framework
runtime states like prerendering or disconnection, not for catching null
reference bugs which should throw immediately rather than silently no-op in
downstream Invoke*/FastInvoke* calls.
In `@src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cs`:
- Around line 175-181: The ReportIfUnexpectedNull method is causing double error
reporting because it cannot distinguish between a null result that came from an
already-handled exception in FastInvoke* methods versus a truly unexpected null
from a valid runtime. The fix requires either: (1) modifying the contract so
that FastInvoke* methods return a result wrapper or sentinel value that
indicates whether the null came from an already-reported error, allowing
ReportIfUnexpectedNull to skip reporting in that case, or (2) moving the null
result validation logic entirely into the FastInvoke* methods themselves so that
ReportIfUnexpectedNull is not needed as a defensive check. Choose the approach
that best fits your architecture and ensure that only the initial interop
failure is reported, not a secondary InvalidOperationException for the same
underlying issue.
In `@src/BlazorUI/Bit.BlazorUI/Utils/BitPageVisibility.cs`:
- Around line 50-59: The Dispose method in BitPageVisibility is performing a
fire-and-forget JS interop call to "BitBlazorUI.PageVisibility.dispose" which
hides teardown failures and leaves the JS-side listener uncleared. Implement
IAsyncDisposable on the BitPageVisibility class instead of IDisposable, and
replace the Dispose method with a DisposeAsync method. In DisposeAsync, properly
await the JS cleanup operation using js.InvokeAsync instead of the
fire-and-forget js.InvokeVoid call, ensuring the _isInitialized flag is reset
after the async operation completes. This allows Blazor's DI lifecycle to
properly handle the asynchronous disposal of this scoped service.
---
Outside diff comments:
In
`@src/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cs`:
- Around line 20-28: The methods BitExtrasInitScripts and
BitExtrasInitStylesheets are using InvokeVoid() which is a fire-and-forget
operation that returns an immediately completed ValueTask, but the underlying
JavaScript functions return Promises that must be awaited before resources are
fully loaded. Replace InvokeVoid() with InvokeVoidAsync() in both method
implementations to properly await the JavaScript Promise resolution and ensure
callers' await statements actually wait for resource loading to complete.
---
Nitpick comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.ts`:
- Around line 26-30: The console.error call in the catch block of the dispose
method lacks the context prefix used consistently across other components in
this layer. Add the prefix "BitBlazorUI.ColorPicker.dispose:" to the error log
in the catch block (where the console.error(e) statement is located) to match
the logging convention used in BitSwiper.ts, BitSplitter.ts, and other files,
improving debugging clarity.
In
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cs`:
- Around line 62-64: The LastTwoSegments method reduces identifiers to
Class.method form by assuming class names are unique across TypeScript sources,
which could cause false positives if multiple classes share the same name across
different namespaces. Add a comment near the LastTwoSegments call explaining
this assumption and documenting why it is acceptable for the current
single-project scope and typical TypeScript structure, making the tradeoff
explicit for future maintainers.
- Around line 75-79: Ensure that the design tradeoff regarding Promise
delegation propagation is explicitly and clearly documented in
TsPromiseMethodScanner (around lines 19-22), specifically stating that
delegation is propagated from methods with explicit async or : Promise<...>
annotations but NOT from body-scanned heuristics like return fetch(...). Verify
that TsPromiseMethodScannerTests (around lines 135-152) includes tests that
validate this behavior and demonstrate the documented limitations. Additionally,
confirm that the implementation in TsPromiseMethodScanner (lines 101-117)
correctly implements this explicit design choice by only propagating delegation
from explicitly annotated methods while intentionally excluding
heuristically-detected methods.
In
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cs`:
- Around line 130-135: The foreach loop in lines 130-133 unnecessarily copies
each item from `promiseMethods` into the empty `result` collection. Since
`result` starts empty and is only populated with items from `promiseMethods`,
eliminate this redundant loop by directly returning `promiseMethods` instead of
copying its contents to `result`. This removes an unnecessary allocation and
iteration while maintaining the same behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dfc615b4-856f-49f8-b792-bb6df3b8235a
📒 Files selected for processing (59)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AppShell/BitAppShellJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/JsInterop/BitChartJsInterop.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGrid.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/DataGrid/BitDataGridJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/InfiniteScrolling/BitInfiniteScrollingJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownViewer/BitMarkdownViewer.tssrc/BlazorUI/Bit.BlazorUI.Extras/Components/MarkdownViewer/BitMarkdownViewerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PdfReader/BitPdfReaderJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/PhoneInput/BitPhoneInput.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/ProModal/BitProModal.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Extensions/JsInterop/ExtrasJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI.Extras/Scripts/Extras.tssrc/BlazorUI/Bit.BlazorUI.Extras/Services/BitMarkdownService.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/CircularTimePicker/BitCircularTimePickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPickerJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DatePicker/BitDatePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/DateRangePicker/BitDateRangePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUpload.tssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/FileUpload/BitFileUploadJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/OtpInput/BitOtpInputJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBox.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBoxJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Slider/BitSlider.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Carousel/BitCarousel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiper.tssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Swiper/BitSwiperJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Dialog/BitDialog.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Panel/BitPanel.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitter.tssrc/BlazorUI/Bit.BlazorUI/Components/Surfaces/Splitter/BitSplitterJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/Overlay/BitOverlay.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefresh.tssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/PullToRefresh/BitPullToRefreshJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Utilities/SwipeTrap/BitSwipeTrap.tssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/JsInteropConstants.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/PageVisibility.tssrc/BlazorUI/Bit.BlazorUI/Scripts/Swipes.tssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Bit.BlazorUI/Utils/BitPageVisibility.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Bit.BlazorUI.Tests.csprojsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AppShell/BitAppShellTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/FastInvokeSyncContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IJSRuntimeFastExtensionsTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/IsRuntimeInvalidFrameworkContractTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScanner.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Extensions/JsInterop/TsPromiseMethodScannerTests.cs
Comment on lines
+150
to
+155
| // Always signal completion on first render, matching the long-standing behavior: consumers may | ||
| // rely on SetupCompletedCallback firing once the component has rendered regardless of whether the | ||
| // chart setup itself succeeded (e.g. when Config is still null, or when interop was unavailable | ||
| // and the result was swallowed on the WebAssembly fast path). | ||
| await SetupCompletedCallback.InvokeAsync(this); | ||
|
|
There was a problem hiding this comment.
SetupCompletedCallback documentation contradicts its actual firing condition.
The XML documentation (lines 30–33) states the callback fires "when the chart has been setup through interop and the JavaScript chart object is available," but the code now invokes it unconditionally on first render regardless of whether Config is null or setup succeeded. Consumers relying on this callback to signal "chart is ready" will receive false positives when Config is null or when the interop call fails/returns false on WebAssembly's fast path.
Either update the XML docs to match the actual behavior (fires after first render regardless of setup outcome), or move the callback invocation inside the if (Config is not null) block and only invoke it when setup completes successfully.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/BlazorUI/Bit.BlazorUI.Extras/Components/Chart/BitChart.razor.cs` around
lines 150 - 155, The XML documentation for SetupCompletedCallback states it
fires when the chart has been setup through interop and the JavaScript object is
available, but the current implementation invokes it unconditionally on first
render regardless of whether Config is null or setup succeeded. Either move the
await SetupCompletedCallback.InvokeAsync(this) call to execute only when setup
completes successfully by placing it inside the if (Config is not null) block,
or update the XML documentation comments to accurately reflect that the callback
fires unconditionally on first render. The recommended approach is to move the
callback invocation to only fire when setup actually succeeds so that consumers
relying on this callback to signal chart readiness do not receive false
positives.
Comment on lines
+98
to
+104
| [SuppressMessage("Trimming", "IL2075:'this' argument does not satisfy 'DynamicallyAccessedMembersAttribute' in call to target method. The return value of the source method does not have matching annotations.", Justification = "Reflection here only reads a well-known bool property (RemoteJSRuntime.IsInitialized) and a private field (WebViewJSRuntime._ipcSender) by name for host-runtime detection; no members are dynamically invoked or instantiated, and a missing member is handled by treating the runtime as valid, so trimming cannot break this probe.")] | ||
| public static bool IsRuntimeInvalid(this IJSRuntime jsRuntime) | ||
| { | ||
| if (jsRuntime is null) return false; | ||
| // A null runtime can't service interop, so report it as invalid. This lets the no-op paths in | ||
| // InvokeVoid/Invoke/FastInvoke skip the call instead of dereferencing null (the async fallback | ||
| // would otherwise throw ArgumentNullException from the framework's JSRuntimeExtensions). | ||
| if (jsRuntime is null) return true; |
There was a problem hiding this comment.
Don’t treat a null runtime as an “invalid host runtime”.
null here is a caller bug, not a prerender/disconnected framework runtime. Returning true makes every Invoke*/FastInvoke* wrapper silently no-op or hand back defaults, which will hide missing DI/initialization defects and leave components half-initialized instead of failing fast. Keep throwing on null and reserve IsRuntimeInvalid() for real framework runtime states only.
Suggested fix
- if (jsRuntime is null) return true;
+ ArgumentNullException.ThrowIfNull(jsRuntime);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [SuppressMessage("Trimming", "IL2075:'this' argument does not satisfy 'DynamicallyAccessedMembersAttribute' in call to target method. The return value of the source method does not have matching annotations.", Justification = "Reflection here only reads a well-known bool property (RemoteJSRuntime.IsInitialized) and a private field (WebViewJSRuntime._ipcSender) by name for host-runtime detection; no members are dynamically invoked or instantiated, and a missing member is handled by treating the runtime as valid, so trimming cannot break this probe.")] | |
| public static bool IsRuntimeInvalid(this IJSRuntime jsRuntime) | |
| { | |
| if (jsRuntime is null) return false; | |
| // A null runtime can't service interop, so report it as invalid. This lets the no-op paths in | |
| // InvokeVoid/Invoke/FastInvoke skip the call instead of dereferencing null (the async fallback | |
| // would otherwise throw ArgumentNullException from the framework's JSRuntimeExtensions). | |
| if (jsRuntime is null) return true; | |
| [SuppressMessage("Trimming", "IL2075:'this' argument does not satisfy 'DynamicallyAccessedMembersAttribute' in call to target method. The return value of the source method does not have matching annotations.", Justification = "Reflection here only reads a well-known bool property (RemoteJSRuntime.IsInitialized) and a private field (WebViewJSRuntime._ipcSender) by name for host-runtime detection; no members are dynamically invoked or instantiated, and a missing member is handled by treating the runtime as valid, so trimming cannot break this probe.")] | |
| public static bool IsRuntimeInvalid(this IJSRuntime jsRuntime) | |
| { | |
| // A null runtime can't service interop, so report it as invalid. This lets the no-op paths in | |
| // InvokeVoid/Invoke/FastInvoke skip the call instead of dereferencing null (the async fallback | |
| // would otherwise throw ArgumentNullException from the framework's JSRuntimeExtensions). | |
| ArgumentNullException.ThrowIfNull(jsRuntime); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeExtensions.cs`
around lines 98 - 104, Remove the null check from the IsRuntimeInvalid method
that returns true when jsRuntime is null. The method should not treat null as an
invalid runtime state; instead null should propagate naturally as a caller error
to fail fast. Reserve IsRuntimeInvalid() only for detecting real framework
runtime states like prerendering or disconnection, not for catching null
reference bugs which should throw immediately rather than silently no-op in
downstream Invoke*/FastInvoke* calls.
Comment on lines
+175
to
+181
| public static T? ReportIfUnexpectedNull<T>(this IJSRuntime jsRuntime, string identifier, T? result) | ||
| where T : class | ||
| { | ||
| if (result is not null || jsRuntime.IsRuntimeInvalid()) return result; | ||
|
|
||
| ReportError(identifier, new InvalidOperationException("The interop call completed without a result.")); | ||
| return null; |
There was a problem hiding this comment.
ReportIfUnexpectedNull now double-reports real FastInvoke failures.
When FastInvoke* catches JsonException/JSException, it already calls ReportError and returns null. Any caller that then passes that result through this helper turns one interop failure into two reports: the original exception and a second InvalidOperationException. That will inflate alerts/logs and it contradicts the remarks above that only the “valid runtime, null without exception” case should be reported here. This needs a contract that preserves whether the null came from a swallowed error, or the unexpected-null detection needs to move into FastInvoke* itself.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/IJSRuntimeFastExtensions.cs`
around lines 175 - 181, The ReportIfUnexpectedNull method is causing double
error reporting because it cannot distinguish between a null result that came
from an already-handled exception in FastInvoke* methods versus a truly
unexpected null from a valid runtime. The fix requires either: (1) modifying the
contract so that FastInvoke* methods return a result wrapper or sentinel value
that indicates whether the null came from an already-reported error, allowing
ReportIfUnexpectedNull to skip reporting in that case, or (2) moving the null
result validation logic entirely into the FastInvoke* methods themselves so that
ReportIfUnexpectedNull is not needed as a defensive check. Choose the approach
that best fits your architecture and ensure that only the initial interop
failure is reported, not a secondary InvalidOperationException for the same
underlying issue.
Comment on lines
+50
to
+59
| public void Dispose() | ||
| { | ||
| if (_isInitialized) | ||
| { | ||
| _isInitialized = false; | ||
|
|
||
| // Tears down the global visibilitychange listener and resets the JS-side init guard, | ||
| // so the now-disposed _dotnetObj is no longer referenced and a future instance can re-init. | ||
| _ = js.InvokeVoid("BitBlazorUI.PageVisibility.dispose"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "BitPageVisibility.cs" | head -20Repository: bitfoundation/bitplatform
Length of output: 124
🏁 Script executed:
fd -t f "BitPageVisibility\.cs"Repository: bitfoundation/bitplatform
Length of output: 122
🏁 Script executed:
rg "class BitPageVisibility" -A 30 -B 5Repository: bitfoundation/bitplatform
Length of output: 2929
🏁 Script executed:
rg "BitPageVisibility" --type cs -lRepository: bitfoundation/bitplatform
Length of output: 359
🏁 Script executed:
cat -n src/BlazorUI/Bit.BlazorUI/Utils/BitPageVisibility.csRepository: bitfoundation/bitplatform
Length of output: 2109
🏁 Script executed:
rg "BitPageVisibility" -B 5 -A 10 src/BlazorUI/Bit.BlazorUI/Components/Lists/Carousel/BitCarousel.razor.csRepository: bitfoundation/bitplatform
Length of output: 483
🏁 Script executed:
rg "BitPageVisibility" -B 3 -A 10 src/BlazorUI/Bit.BlazorUI/Extensions/IBitBlazorUIServiceCollectionExtensions.csRepository: bitfoundation/bitplatform
Length of output: 305
🏁 Script executed:
rg "\.Dispose\(\)" src/BlazorUI/Bit.BlazorUI/Components/Lists/Carousel/BitCarousel.razor.cs -B 3 -A 3Repository: bitfoundation/bitplatform
Length of output: 736
🏁 Script executed:
rg "_pageVisibility" src/BlazorUI/Bit.BlazorUI/Components/Lists/Carousel/BitCarousel.razor.cs -B 2 -A 2Repository: bitfoundation/bitplatform
Length of output: 613
🏁 Script executed:
cat -n src/BlazorUI/Bit.BlazorUI/Components/Lists/Carousel/BitCarousel.razor.cs | grep -A 20 "public async Task"Repository: bitfoundation/bitplatform
Length of output: 1550
Implement IAsyncDisposable to properly await JS teardown instead of fire-and-forget in Dispose.
On line 58, _ = js.InvokeVoid("BitBlazorUI.PageVisibility.dispose") is a fire-and-forget call that discards the ValueTask. This hides teardown failures and can leave the JS-side listener uncleared. Since Init() already uses async JS interop, move the JS cleanup to DisposeAsync() and let Blazor's DI lifecycle handle async disposal for this scoped service.
Proposed fix
-public class BitPageVisibility(IJSRuntime js) : IDisposable
+public class BitPageVisibility(IJSRuntime js) : IDisposable, IAsyncDisposable
{
private bool _isInitialized;
private DotNetObjectReference<BitPageVisibility>? _dotnetObj;
@@
public void Dispose()
{
+ _dotnetObj?.Dispose();
+ }
+
+ public async ValueTask DisposeAsync()
+ {
if (_isInitialized)
{
_isInitialized = false;
- _ = js.InvokeVoid("BitBlazorUI.PageVisibility.dispose");
+ try
+ {
+ await js.InvokeVoid("BitBlazorUI.PageVisibility.dispose");
+ }
+ catch (JSDisconnectedException) { }
}
-
_dotnetObj?.Dispose();
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/BlazorUI/Bit.BlazorUI/Utils/BitPageVisibility.cs` around lines 50 - 59,
The Dispose method in BitPageVisibility is performing a fire-and-forget JS
interop call to "BitBlazorUI.PageVisibility.dispose" which hides teardown
failures and leaves the JS-side listener uncleared. Implement IAsyncDisposable
on the BitPageVisibility class instead of IDisposable, and replace the Dispose
method with a DisposeAsync method. In DisposeAsync, properly await the JS
cleanup operation using js.InvokeAsync instead of the fire-and-forget
js.InvokeVoid call, ensuring the _isInitialized flag is reset after the async
operation completes. This allows Blazor's DI lifecycle to properly handle the
asynchronous disposal of this scoped service.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #9917
Summary by CodeRabbit